C#: Fix cs/web/xss false positive on Razor tag-helper attribute values by felickz · Pull Request #22628 · github/codeql

felickz · 2026-09-18T21:28:29Z

Summary

cs/web/xss currently flags RazorPageBase.WriteLiteral(...) calls that the Razor source generator emits for the value of an HTML attribute on an element that also carries a tag helper (for example, asp-for="Model.Something"). This is a false positive: the Razor codegen brackets these calls between BeginWriteTagHelperAttribute() / EndWriteTagHelperAttribute(), which capture the value into an internal buffer rather than writing it directly to the response. It is not a real XSS sink.

Example

Given a controller action that binds user-provided input to a view model:

[HttpPost]
public IActionResult UpdateProfile(ProfileViewModel model)
{
    if (!ModelState.IsValid)
        return View(model);
    ...
}

and a Razor Pages/MVC view that renders one of its properties through a tag helper attribute:

<input asp-for="DisplayName" type="text" value="@Model.DisplayName" class="textEntry">

cs/web/xss previously reported:

User-provided value flows to here and is written to HTML or JavaScript: Microsoft.AspNetCore.Mvc.Razor.RazorPageBase.WriteLiteral() method.

This is a false positive. The value assigned to the asp-for attribute is captured into an internal string buffer via matching BeginWriteTagHelperAttribute() / EndWriteTagHelperAttribute() calls generated around the WriteLiteral call, so it becomes a tag helper attribute value rather than being written directly to the response as page markup.

Fix

AspNetCore.qll: add getBeginWriteTagHelperAttributeMethod() / getEndWriteTagHelperAttributeMethod() to MicrosoftAspNetCoreMvcRazorPageBase.
Html.qll: add isBracketedForTagHelperAttribute() and use it to exclude bracketed WriteLiteral calls from MicrosoftAspNetRazorPageWriteLiteralSink. The predicate requires beginCall, writeLiteral, endCall to appear (in that order) in the same basic block, with no other Begin/EndWriteTagHelperAttribute call in between on either side, and all three calls to share an implicit this receiver, so an unrelated bracket (on this or another page instance) can't "adopt" an unbracketed call.

Test coverage

Added RazorTagHelperAttribute.cshtml / .cshtml.g.cs to the existing Security Features/CWE-079/XSS test, covering:

A bracketed WriteLiteral(model) (must not alert, suppressed FP).
An unbracketed WriteLiteral(model) (must alert, positive control).
Two independent bracket pairs back-to-back in one basic block (must not leak into each other).
A bare WriteLiteral(model) sandwiched between two unrelated brackets (must still alert).

Full Security Features/CWE-079 suite (6 tests) passes with no regressions. Also validated end-to-end against a small standalone ASP.NET Core Razor Pages reproduction project mirroring the example above: the false positive is no longer reported, while a genuine Html.Raw-based positive control in the same project remains reported.

Change note

Added csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md (category: majorAnalysis).

Razor's source generator brackets WriteLiteral calls that populate an HTML attribute value on a tag-helper-enabled element (e.g. �sp-for) between BeginWriteTagHelperAttribute()/EndWriteTagHelperAttribute() calls. The captured text is buffered internally and HTML-attribute- encoded before being rendered, so it is not a real XSS sink, but cs/web/xss previously flagged it as one. - Add getBeginWriteTagHelperAttributeMethod() / getEndWriteTagHelperAttributeMethod() to MicrosoftAspNetCoreMvcRazorPageBase. - Add isBracketedForTagHelperAttribute() to Html.qll and use it to exclude bracketed WriteLiteral calls from MicrosoftAspNetRazorPageWriteLiteralSink, using same-basic-block, immediately-adjacent-bracket matching so unrelated bracket pairs cannot "adopt" an unbracketed call. - Add RazorTagHelperAttribute.cshtml(.g.cs) test coverage: a suppressed bracketed write, an unbracketed positive control, two independent brackets in one basic block, and a bare write sandwiched between brackets. - Add change note (majorAnalysis). Diagnosed and validated against the real customer reproduction that motivated this fix (field-security-codeql#257 / github#261): the false positive (ChangeAccountInfo.cshtml) is no longer reported, while the genuine Html.Raw-based positive control (ProfileSummary.cshtml) remains reported. Full CWE-079 test suite (6 tests) passes with no regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot Code Review correctly flagged that isBracketedForTagHelperAttribute never related the receivers of beginCall/writeLiteral/endCall, so a bracket on one page instance could theoretically be mistaken for a bracket around a WriteLiteral call on a different page (e.g. otherPage.BeginWriteTagHelperAttribute(); this.WriteLiteral(model); otherPage.EndWriteTagHelperAttribute();). Require all three calls to have an implicit his qualifier, which is how the Razor source generator always emits them, guaranteeing they act on the same page instance. Re-verified: XSS.ql compiles, all 6 CWE-079 tests pass, and the real customer reproduction database still shows the FP suppressed and the genuine Html.Raw positive control still reported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Shortened to a single terse sentence, matching the depth/style of other recent change-notes (one bullet, no implementation detail), and called out the ASP.NET Core Razor Pages/MVC scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

The documentation incorrectly promises downstream HTML encoding instead of stating that the call only writes to a temporary buffer.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Avoid assuming buffered tag-helper values are HTML-attribute-encoded · New
Correct release note overstating tag-helper encoding guarantees · New

What changed in this PR

Prevents false-positive C# XSS alerts for buffered Razor tag-helper attribute writes.

Changes:

Models Razor tag-helper attribute buffering methods.
Excludes correctly bracketed WriteLiteral calls from direct XSS sinks.
Adds regression fixtures and a change note.

File	Description
`Html.qll`	Detects buffered `WriteLiteral` calls.
`AspNetCore.qll`	Models tag-helper buffering methods.
`RazorTagHelperAttribute.cshtml`	Provides source-map fixture.
`RazorTagHelperAttribute.cshtml.g.cs`	Adds generated-code test scenarios.
`XSS.expected`	Updates expected query results.
Change note	Documents the analysis change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot Code Review correctly pointed out that bracketing between BeginWriteTagHelperAttribute()/EndWriteTagHelperAttribute() only proves the value is captured into a buffer rather than written directly to the response; it does not, by itself, guarantee that every tag helper later HTML-attribute-encodes that buffer. Reworded the doc comments and the change note to justify the exclusion on "not a direct write to the response" rather than on assumed downstream encoding. No logic change; XSS.ql compiles and the CWE-079/XSS test still passes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟢 Approval recommended

The sink exclusion is narrowly constrained by receiver, control-flow order, and bracket boundaries, with appropriate regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (2)

Avoid assuming buffered tag-helper values are HTML-attribute-encoded
Correct release note overstating tag-helper encoding guarantees

michaelnebel

Thank you very much for doing this @felickz !
I have added a couple of minor comments.
In the description, you mention "ASP.NET Core Razor Pages reproduction project mirroring the example". Which project is that? Perhaps, we should add it to our DCA suite.

michaelnebel · 2026-09-22T07:08:18Z

+      i2 < j
+    ) and
+    not exists(int k2, Call other |
+      (
+        other = page.getBeginWriteTagHelperAttributeMethod().getACall() or
+        other = page.getEndWriteTagHelperAttributeMethod().getACall()
+      ) and
+      writeLiteral.getBasicBlock().getNode(k2) = other.getControlFlowNode() and
+      j < k2 and
+      k2 < k


Suggested change

i2 < j

) and

not exists(int k2, Call other |

(

other = page.getBeginWriteTagHelperAttributeMethod().getACall() or

other = page.getEndWriteTagHelperAttributeMethod().getACall()

) and

writeLiteral.getBasicBlock().getNode(k2) = other.getControlFlowNode() and

j < k2 and

k2 < k

i2 < k

Shouldn't this suffice as well?
The difference is that we don't check that there is both another begin/end between in ]i-j[ AND ]j-k[, but we only check there is no other begin/end is the most narrow begin/end around WriteLiteral.

michaelnebel · 2026-09-22T07:10:56Z

+    endCall.hasImplicitThisQualifier() and
+    writeLiteral.getBasicBlock().getNode(i) = beginCall.getControlFlowNode() and
+    writeLiteral.getBasicBlock().getNode(j) = writeLiteral.getControlFlowNode() and
+    writeLiteral.getBasicBlock().getNode(k) = endCall.getControlFlowNode() and


Perhaps introduce a BasicBlock bb as a part of the exists with bb = writeLiteral.getBasicBlock(), then it is a bit easier to see that no other basic blocks are used by mistake.

Kamil-0425

Setup

felickz and others added 3 commits September 18, 2026 10:33

github-actions Bot added C# documentation labels Sep 18, 2026

felickz marked this pull request as ready for review September 18, 2026 21:33

felickz requested a review from a team as a code owner September 18, 2026 21:33

Copilot AI balanced review requested due to automatic review settings September 18, 2026 21:33

Copilot started reviewing on behalf of felickz September 18, 2026 21:34 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll Outdated

Comment thread csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md Outdated

felickz requested a balanced review from Copilot September 21, 2026 20:13

Copilot started reviewing on behalf of felickz September 21, 2026 20:14 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

michaelnebel reviewed Sep 22, 2026

View reviewed changes

Kamil-0425 reviewed Sep 22, 2026

View reviewed changes

Navigation Menu

C#: Fix cs/web/xss false positive on Razor tag-helper attribute values - #22628

C#: Fix cs/web/xss false positive on Razor tag-helper attribute values#22628
felickz wants to merge 4 commits into
github:mainfrom
forks-felickz:felickz-csharp-razor-tag-helper-xss-fp

felickz commented Sep 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

michaelnebel left a comment

Uh oh!

michaelnebel Sep 22, 2026

Uh oh!

michaelnebel Sep 22, 2026

Uh oh!

Kamil-0425 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

felickz commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Example

Fix

Test coverage

Change note

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

michaelnebel Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Kamil-0425 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felickz commented Sep 18, 2026 •

edited

Loading